-
Notifications
You must be signed in to change notification settings - Fork 792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(logger): provide getLogger helper and remove NoopLogger class implementation [#1754] #1844
Conversation
|
Codecov Report
@@ Coverage Diff @@
## master #1844 +/- ##
==========================================
- Coverage 92.62% 92.26% -0.36%
==========================================
Files 174 159 -15
Lines 6045 5199 -846
Branches 1286 1107 -179
==========================================
- Hits 5599 4797 -802
+ Misses 446 402 -44
|
df070b7
to
3b779b3
Compare
I don't think it is a good idea to force people to use sdk everywhere to avoid using |
My understanding is that the API is supposed to be interface only and therefore should no have any implementations... I can move the getLogger() (and remove ConsoleLogger fallback) to API, but it seems to be the wrong direction (IMHO anyway).
All packages (except instrumentation) already have a dependency on core, and based on the above this seems correct. Technically, if a package doesn't want to have external links (NoopLogger for instrumentation) then it probably should be handling an undefined logger rather than requiring a NoopLogger() implementation etc. (This seems like a larger discussion)
Sure, but there doesn't currently to be any minification step as part of the build -- this might take a while before I can get enough cycles to do this as well.
I partially agree, the only reason I added was because of the existing usage -- a better approach I think would be if have a "default" logger as the fallback, which this provides a possible platform to "add" a setDefaultLogger() implementation. |
The API contains also noop implementations for e.g There were quite some PRs in the past to move parts from core to api/context/instrumentation. |
Removing ConsoleLogger changes (added issue #1847) and moving getLogger to api package |
3b779b3
to
66e1b86
Compare
As discussed in the SIG, I'm going to abandon this PR and change it into providing a "diag" constant at the api level and then change the code to use this value
|
…ient-sqs versions using the AWS JSON 1.0 protocol (open-telemetry#1844) Versions 3.446.0 and later switched to a new JSON protocol. https://github.com/aws/aws-sdk-js-v3/blob/main/clients/client-sqs/CHANGELOG.md#34460-2023-11-08 Refs: open-telemetry/opentelemetry-js-contrib#1838 (comment)
Which problem is this PR solving?
Short description of the changes
getLogger(logger?: Logger | null)
to core